Skip to content

Checks selected variable is in choices available. #254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Checks selected variable is in choices available. #254

wants to merge 4 commits into from

Conversation

llrs-roche
Copy link
Contributor

Pull Request

Fixes #253

This is a very minimal PR to help users use choices_selected and value_choices together (see test added).

The nested if is a matter of style to avoid having a line above 120 characters which would be flagged by the linter.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 11, 2025 •
edited
Loading

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@github-actions GitHub Actions
Copy link
Contributor

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------------
R/call_utils.R                      156     124  20.51%   19-28, 65, 67, 69, 102-345
R/check_selector.R                   33       0  100.00%
R/choices_labeled.R                 157      27  82.80%   68, 74, 79, 86, 102, 220-224, 228-233, 357-358, 360, 366, 393-400
R/choices_selected.R                 84      11  86.90%   219-247, 284
R/column_functions.R                  3       3  0.00%    15-18
R/data_extract_datanames.R           30       8  73.33%   16-20, 83-85
R/data_extract_filter_module.R      105      47  55.24%   95-108, 111-112, 114-131, 147-166
R/data_extract_module.R             298      67  77.52%   128, 133, 150, 153-158, 160, 179-182, 212-258, 497, 502, 679, 690-691, 769-774
R/data_extract_read_module.R        137       7  94.89%   34, 39-41, 43, 138, 155
R/data_extract_select_module.R       32      18  43.75%   29-46
R/data_extract_single_module.R       60       2  96.67%   30, 43
R/data_extract_spec.R                32       0  100.00%
R/delayed_choices.R                  34       6  82.35%   87, 96-100
R/filter_spec.R                     186       1  99.46%   280
R/format_data_extract.R              16       1  93.75%   48
R/get_dplyr_call.R                  297       0  100.00%
R/get_merge_call.R                  278      29  89.57%   32-38, 49, 215-224, 391, 407-419
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   17-18
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  134       6  95.52%   123, 249-253
R/merge_expression_module.R          60      11  81.67%   161-166, 184, 362-367
R/Queue.R                            23       0  100.00%
R/resolve_delayed.R                  16       4  75.00%   78-81
R/resolve.R                         115      45  60.87%   48, 182-288
R/select_spec.R                      64       8  87.50%   99, 179-186
R/utils.R                            37      24  35.14%   33-46, 174-187
R/zzz.R                               3       3  0.00%    2-4
TOTAL                              2408     454  81.15%

Diff against main

Filename                          Stmts    Miss  Cover
------------------------------  -------  ------  -------
R/choices_labeled.R                  +4       0  +0.45%
R/choices_selected.R                 +3       0  +0.49%
R/data_extract_filter_module.R       +3       0  +1.32%
R/delayed_choices.R                 +34      +6  +82.35%
R/resolve.R                          +2      +1  -0.19%
TOTAL                               +46      +7  +0.06%

Results for commit: c33e92d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 11, 2025 •
edited
Loading

Unit Tests Summary

  1 files   24 suites   6s ⏱️
194 tests 194 ✅ 0 💤 0 ❌
698 runs  698 ✅ 0 💤 0 ❌

Results for commit c33e92d.

♻️ This comment has been updated with latest results.

@m7pr
Copy link
Contributor

m7pr commented Feb 11, 2025

@llrs-roche would you be able to share a tmg example where it didnt fail with a warning and now it fails, and a print screen of the current and expected behavior? Thanks

@llrs-roche
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@chlebowa
Copy link
Contributor

NB there is an intersect call somewhere down the line that prevents this situation from raising an error and this change makes it redundant.

I myself don't believe it's worth it to make this situation an error unless you go through the entire package and change similar places consistently but then I'm not an author here 😉

@llrs-roche
Copy link
Contributor Author

@m7pr @chlebowa I added a test case showing that this didn't fail previously at least with just teal.transform code:

choices_selected(
  value_choices("ADTTE", "PARAMCD", "PARAM"),
  "OS"
)

The current expected behavior is to fail if you requests "OS" but only have "PARAMCD" as variable.
I'm not sure the intersect path was used before, it might be this fix is on the wrong place. The change was relatively easy and I am willing to go and change similar places consistently (if it helps and within priorities). Or find a better place to fix this.

@chlebowa
Copy link
Contributor

Sorry for not being specific. What I was trying to say is the current design (I believe) is to not treat this situation as an error, which is why your added test didn't fail. This is a design choice that is reflected throughout the package.

This change alters that design choice but only for this particular case. If you think this is warranted (with which I respectfully disagree), you should probably apply it in other places as well.

I may have been wrong about the intersect (only looked at the code now), please forgive the confusion. My main point stands.

@llrs-roche
Copy link
Contributor Author

Many thanks @chlebowa for clarifying your opinion. I am not as familiar with the package as you. Let's see what the reviewers or @gogonzo says.

gogonzo
gogonzo requested changes Feb 11, 2025
View reviewed changes
R/choices_selected.R
Comment on lines +155 to +159
if (is.character(selected) && !inherits(choices, "delayed_variable_choices")) {
if (!all(selected %in% choices$var_choices) || !all(selected %in% choices$var_label)) {
stop("Selected, '", selected, "' is not in the available choices.")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to determine this at this point? If choices are delayed, we can't guess if selected is a subset of the chocies.

@gogonzo
Copy link
Contributor

gogonzo commented Feb 20, 2025

@llrs-roche Let's close it and maybe address the issue here #255

@llrs-roche
Copy link
Contributor Author

I'll close but that might make increase the scope of #255.

@llrs-roche llrs-roche closed this Feb 21, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
@insights-engineering-bot insights-engineering-bot deleted the selected_value_choices@main branch May 18, 2025 03:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gogonzo gogonzo gogonzo requested changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Question]: Does choices_selected check if selected is in choices for value_choices?
4 participants
@llrs-roche @m7pr @chlebowa @gogonzo